Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streamlined and unified opacity methods #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

victor-homyakov
Copy link
Contributor

Currently methods for getting/setting opacity have different and inefficient workflow:

  • in IE w/o opacity support getStyle_IE invokes getOpacity_IE (but only after useless operations like normalizeStyleName_IE())
  • in other browsers getOpacity invokes getStyle (doing useless operations like normalizeStyleName('opacity'))

Also setOpacity_IE and getOpacity_IE check STANDARD_CSS_OPACITY_SUPPORTED each time they are invoked (not good for animation performance).

Currently methods for getting/setting opacity have different workflow:

- in IE w/o opacity support `getStyle_IE` invokes `getOpacity_IE`
- in other browsers `getOpacity` invokes `getStyle` (doing unneeded operations like `normalizeStyleName('opacity')`)

Also `setOpacity_IE` and `getOpacity_IE` check `STANDARD_CSS_OPACITY_SUPPORTED` each time they are invoked (not good for animation performance).
@savetheclocktower
Copy link
Collaborator

Hey, can I ask a favor? I want to apply this, but before I do, I need to make absolutely sure that this works in:

  1. IE 6 (which supports only the filter syntax)
  2. IE 7–9 (which support both filter and standard CSS opacity)
  3. IE 10 (which supports opacity but removes support for filter)

If you can assure me that the tests pass in all three of the above (for #2, testing any one of IE 7, 8, or 9 works for me), then I'll merge this. I'm very cautious about this code because I last touched it to add support for IE 10, and it was painstaking surgery to arrive at something that worked everywhere.

@victor-homyakov
Copy link
Contributor Author

  1. I've ran Prototype opacity unit tests (in dom_test.html) in IE Tester (IE6, 7, 8, 9), and in standalone IE7, IE8, IE9, IE10 (on different PCs), and also in recent versions of Chrome, Firefox, Opera - OK (to be absolutely correct, I've got some test failures, e.g. in testGetElementsByClassName in IE6-7-8, testElementScrollTo in Chrome and Firefox, testElementSetStyle in Opera, testToQueryString in all browsers, many of selector_test.html in IEs, but those tests are not relevant to the subject).
  2. I've ran Scriptaculous unit and functional tests in the abovementioned browsers. Opacity in effects and draggables works fine (well, almost fine - all IEs including IE10 have smoothness/performance problems).
  3. I am using this code in production (24x7 cross-browser intranet web app) since at least 28.06.2012. Strictly speaking, I've started this code after profiling my app in IE8-9, then used it for a while in production, and only now made pull request.

@victor-homyakov
Copy link
Contributor Author

There is another opacity-related issue, now in Opera: https://prototype.lighthouseapp.com/projects/8886/tickets/1282-latest-prototypejs-from-github-repo-fails-in-opera - should I make another pull request or include in this one?

@savetheclocktower
Copy link
Collaborator

I'd say make another one. I'll merge this one soon enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants